Infrastructure/11740 playwright email reporting#12330
Conversation
…notations and a new mu-plugin.
…setup' into infrastructure/11740-playwright-email-reporting.
…40-playwright-email-reporting.
|
Size Change: 0 B Total Size: 2.31 MB ℹ️ View Unchanged
|
…it consistent and repeatable across all tests.
…40-playwright-email-reporting.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // Ensure every notice after the first notice has a margin | ||
| // so the notices don't render with no gap. | ||
| // |
There was a problem hiding this comment.
Stylelint blamed this and insisted it be removed. 🤷♂️
…40-playwright-email-reporting.
…40-playwright-email-reporting.
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Thanks @eugene-manuilov, this looks great overall, just a few things to address. Also note there are a few other comments from GitHub + annotations from JS Lint to review.
| update_user_option( | ||
| get_current_user_id(), | ||
| 'googlesitekit_site_verified_meta', | ||
| 'verified' | ||
| ); | ||
|
|
||
| $settings = get_option( Search_Console_Settings::OPTION ); | ||
| $settings['propertyID'] = 'http://localhost:9002'; | ||
| update_option( Search_Console_Settings::OPTION, $settings ); |
There was a problem hiding this comment.
Can we not use filters for these instead? Otherwise we're running these updates on every request. I think this is how we did it before, no?
There was a problem hiding this comment.
Slightly updated it to update the search console settings only once. Re site verification, not sure why but it works only when i do it like that. If I switch to the filter hook, it stops working.
There was a problem hiding this comment.
Ah ok. I see we do it like that in our current E2E utility but it happens in a REST endpoint we call rather than on every request.
| * | ||
| * @param array $http_client_config Guzzle HTTP client configuration array. | ||
| */ | ||
| $http_client_config = apply_filters( 'googlesitekit_http_client_config', $http_client_config ); |
There was a problem hiding this comment.
I'd rather not add a filter just for use in E2E, but I'm not sure there is another way. Alternatively, we could conditionally only apply it in tests/e2e – WDYT?
There was a problem hiding this comment.
Unfortunately, i can't find another way how to do it.
There was a problem hiding this comment.
After thinking about this some more, I think we can overload the access token value which we're already controlling with a filter in tests to use a plain test-access-token. We can essentially append/embed additional metadata (or just pass json in this field, encoded if necessary) and then we can pass whatever we want through to the service and it will be contained in tests. This would be extensible to other values or configuration we want to send through as well. Yes, it gets encrypted (this is necessary) but it will be decrypted before being set in the authorization header. I think this should work since we should also never need to use a fixture for an unauthenticated response. Let me know what you think 👍
There was a problem hiding this comment.
Nice idea, updated.
There was a problem hiding this comment.
Actually, it turned out if we do it this way, auto-authorization stops working. I had to revert it to the previous approach with the filter. I think we can create a ticket to address this challenge separately.
| try { | ||
| startServers(); | ||
| } catch ( err ) { | ||
| global.console.error( | ||
| 'Failed to start HTTPS server:', | ||
| err instanceof Error ? err.message : String( err ) | ||
| ); | ||
| global.console.log( 'Continuing with HTTP only.' ); | ||
| } |
There was a problem hiding this comment.
startServers starts both http and https but there's no guarantee that http started successfully in the catch here, no?
There was a problem hiding this comment.
Yes, but that is correct. I don't really think we will actually need the http version though, because all APIs have https URLs.
There was a problem hiding this comment.
My point is that the messages here imply that http will always start but we can't say for sure. It would be better to just say that there was an error starting the servers and not claim that http will still work or that the error was from starting https specifically.
There was a problem hiding this comment.
Removed extra logging
…40-playwright-email-reporting.
…40-playwright-email-reporting.
…40-playwright-email-reporting.
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Thanks @eugene-manuilov this one is almost there, back to you for a few final things. See also my comment/replies on previous threads which aren't listed below.
…40-playwright-email-reporting.
…40-playwright-email-reporting.
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Great, thanks @eugene-manuilov !
There is one unrelated JS test failing for the AMPContainerSelect which is unstable and we can fix in a follow up.
3 participants
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist